Remove key_t #12246
Remove key_t #12246
Conversation
They had the unnecessary difference that for runtime the key was an alias. Advantage of unifying: - Usage is the unified (e.g. get_property always needs the key). - Simplifies implementation (future PR) which improves compilation speed
Unnecessary. Simplifies defining properties.
|
Draft because part 1 (#12245) should be merged first. Otherwise ready for review. Only review extra commits beyond part 1. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
rolandschulz
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
| template<typename V, typename O> struct is_property_value_of<V, O, std::void_t<typename V::key_t>> : | ||
| is_property_key_of<typename V::key_t, O> {}; | ||
| template<typename V> struct is_property_value<V, std::void_t<key_from_value<V>>> : //key_from_value is exposition only | ||
| is_property_key<key_from_value<V>> {}; |
There was a problem hiding this comment.
Is is_property_key actual useful? This could instead just check that it is of type property_value<T...>. And if we remove is_property_key properties don't need to set that trait. And I'm only aware of useful usage of is_property_key_of. Why would a user care whether something is a property key rather than only whether it is a valid property for a certain usage?
There was a problem hiding this comment.
I think the constraint for properties::has_property and properties::get_property requires us to know whether a type is a property key. How do we do that without the is_property_key trait?
There was a problem hiding this comment.
You're right the trait is useful for that and in that case we might as well expose it in case a user has some use for it. And we can still remove the need for the each property to set the trait in the implementation. We can instead e.g. check PropertyToKind.
rolandschulz
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
|
@rolandschulz I forgot to set myself busy in GH, but I'll be off until Jan 3rd. If a review is required before that, please, ask someone else from runtime reviewers. |
|
@rolandschulz I have seen in #12245 that we are not sure if this change should go forward, or at least that's my understanding. Should I review this PR or it's better to wait until it's clear we want to go forward with this change? |
Feel free to wait until #12245 is resolved. |
|
Part of #13669 |
Unnecessary. Simplifies defining properties.
Part 2 of simplifying properties.